Add optional shape enums in sdk by thswansonfxg · Pull Request #870 · foxglove/foxglove-sdk

thswansonfxg · 2026-02-11T22:25:19Z

Changelog

Add point_style optional enum field to LocationFix schema, allowing users to customize the marker style (dot, diamond, square, plus, cross, pin) when visualizing locations on a map.

Docs

The generated schemas/README.md includes the new PointStyle enum and point_style field documentation. No additional docs PR is needed.

Description

This PR adds a new PointStyle enum and optional point_style field to the LocationFix schema across all SDK bindings (Rust,
Python, C++, C, TypeScript, ROS, Protobuf, FlatBuffer, JSON Schema, OMG IDL).

Changes:

New PointStyle enum with values: DOT (default, 0), DIAMOND (1), SQUARE (2), PLUS (3), CROSS (4), PIN (5)
point_style field on LocationFix — optional in all bindings (Option in Rust, std::optional in C++, *const pointer in C, Option in Python)
Generator fix (generatePyclass.ts) — Python SDK codegen was not handling optional enum fields, generating field as i32 instead of field.map(|s| s as i32)
Generator fix (generateSdkCpp.ts) — Support multiple enums per parent schema, handle optional enum to conversion with nullptr for absent values
Generator fix (generateSdkRustCTypes.ts) — Optional enum fields in C FFI now use pointer types for proper nullability
Serde support (schemas.rs) — Added serde_enum_mod_optional! macro for serializing/deserializing Option enum fields as string names in JSON and raw i32 in binary formats, with round-trip tests
Proto gen fix (foxglove_proto_gen) — Detects proto3_optional enum fields and emits the optional serde macro variant

https://linear.app/foxglove/issue/ERT-1612/add-optional-attribute-to-control-point-shape-in-locationfix-messages

linear · 2026-02-11T22:25:22Z

ERT-1612 add optional attribute to control point shape in locationfix messages

…ptional

claude · 2026-02-12T22:27:28Z

Claude finished @thswansonfxg's task in 2m 23s —— View job

Summary

This PR adds a new PointStyle enum and optional point_style field to the LocationFix schema across all SDK bindings (Rust, Python, C++, C, TypeScript, ROS, Protobuf, FlatBuffer, JSON Schema, OMG IDL). The implementation includes generator fixes for handling optional enum fields correctly in Python, C++, and C FFI, plus a new serde macro for optional enum serialization in Rust.

The PR addresses the issues raised by Cursor Bugbot in earlier reviews:

✅ C FFI optional enum: Now uses *const FoxglovePointStyle pointer type for nullability
✅ C++ reinterpret_cast UB: Now uses arena allocation + static_cast instead of reinterpret_cast
✅ Missing serde(default): Added to handle missing keys during JSON deserialization

Blockers

None

Suggestions

1. PR description should use "Fixes:" format for Linear link

The Linear issue link at the bottom of the PR description should use the Fixes: prefix format per the PR template:

Fixes: [ERT-1612](https://linear.app/foxglove/issue/ERT-1612/add-optional-attribute-to-control-point-shape-in-locationfix-messages)

2. Docs section is empty

The PR template asks for documentation updates or a tracking ticket. Since this adds a user-facing schema field, consider:

Linking to a docs PR or ticket, or
Noting "None" explicitly if the schema README update is sufficient

3. Consider adding a test for the C FFI optional enum round-trip

The Rust serde tests (test_location_fix_json_roundtrip_with_point_style, etc.) are good, but there's no explicit test verifying the C FFI *const FoxglovePointStyle → Option<i32> conversion handles both nullptr (None) and valid pointer (Some) cases correctly. The conversion at c/src/generated_types.rs:3303 looks correct:

point_style: unsafe { self.point_style.as_ref() }.map(|&v| v as i32),

But an integration test would catch regressions if the generator changes.

4. Test fixture update is good, but snapshot diffs are noisy

The test fixtures now include field_optional_enum which exercises the new code paths. The snapshot diffs show whitespace/indentation changes unrelated to the feature - this appears to be a formatting normalization. Not a blocker, just noting it makes the diff harder to review.

5. Minor: `color` field uses `reinterpret_cast` in C++ but `point_style` uses arena allocation

Looking at cpp/foxglove/src/schemas.cpp:1760:

dest.color = src.color ? reinterpret_cast<const foxglove_color*>(&*src.color) : nullptr;

The color field (a nested Color struct) uses reinterpret_cast while point_style (an enum) correctly uses arena allocation. This is fine because Color is in the copyTypes set (same memory layout in C++ and C), but the inconsistency might confuse future readers. A comment explaining why enums need arena allocation while some structs don't would help.

Overall this is solid work - the optional enum handling across all the SDK bindings is thorough, and the fixes for the UB and serde issues are correct. 👍

…-trip tests

c/src/generated_types.rs

typescript/schemas/src/internal/generateSdkCpp.ts

…te-to-control-point-shape-in-locationfix

cpp/foxglove/src/schemas.cpp

…_cast

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

rust/foxglove_proto_gen/src/lib.rs

…ring deserialization

jtbandes

This feels like a good direction, I'm just not sure if I have quite enough expertise on the Rust and C++ (particularly Arena) side of things to give a definitive stamp 😬

jtbandes · 2026-02-13T00:29:20Z

rust/foxglove/src/schemas.rs

+///
+/// Uses string names for human-readable formats (JSON) and i32 for binary formats.
+#[cfg(feature = "serde")]
+macro_rules! serde_enum_mod_optional {


@eloff @ericmlujan could you folks review this part? I'm not familiar with this code at all and @gasmith is out -- I do wonder if there's a way we could combine this with the non-optional macro rather than having two whole macros for it? but not sure how feasible that suggestion is

I'm certainly not confident enough in Rust macros to combine them nicely, especially for SDK level code. This was done because it was easier for me to understand and easier for me to read.

Super happy to change this if someone better at rust has some ideas.

jtbandes · 2026-02-13T00:37:38Z

typescript/schemas/src/internal/generateSdkRustCTypes.ts

            return `${fieldName}: self.${fieldName}`;
          case "enum":
+            if (field.optional) {
+              return `${fieldName}: unsafe { self.${fieldName}.as_ref() }.map(|&v| v as i32)`;


is this actually unsafe? I think this is for someone who knows more Rust than me to answer

I think yes because self.point_style is a *const FoxglovePointStyle — a raw pointer in the C FFI struct.

But we also talked about this in person. Someone who knows rust better might need to stamp this.

jtbandes · 2026-02-13T00:49:29Z

typescript/schemas/src/internal/generateSdkCpp.ts

      case "enum":
+        if (field.optional) {
+          const cEnumType = `foxglove_${toSnakeCase(field.type.enum.name)}`;
+          return `if (src.${srcName}) { auto* ptr = arena.alloc<${cEnumType}>(1); *ptr = static_cast<${cEnumType}>(*src.${srcName}); dest.${dstName} = ptr; } else { dest.${dstName} = nullptr; }`;


I am wondering if this whole thing could possibly be replaced with dest.${dstName} = reinterpret_cast<const ${cEnumType}*>(src.${srcName})?

I'm not sure if that would actually work or be safe - it just came to mind since the generated code looked a bit complicated and I think these types are compatible.

If that doesn't work, it might be nice to actually put some line breaks in this string so it's more readable here (though it does get auto-formatted in the generated files)

That was implemented because of this cursor comment here: #870 (comment)

Further reading says that maybe its totally safe? I am happy to change this, but its being flagged because of garbage bytes and the potential of undefined behavior

ah ok, I missed that the sizes are different, I guess that makes sense

jtbandes · 2026-02-13T00:55:52Z

typescript/schemas/src/internal/schemas.ts

+  name: "PointStyle",
+  description: "Style of point used for map visualization",
+  parentSchemaName: "LocationFix",
+  protobufEnumName: "PointStyle",


I wonder if we should do something like

Suggested change

name: "PointStyle",

description: "Style of point used for map visualization",

parentSchemaName: "LocationFix",

protobufEnumName: "PointStyle",

name: "MapPointStyle", //(or LocationFixPointStyle?)

description: "Style of point used for map visualization",

parentSchemaName: "LocationFix",

protobufEnumName: "PointStyle",

?

Although it's also making me realize that what is currently called "protobufEnumName" should maybe be used for any language where we nest enums inside the schemas (which it seems includes c++ too)...

I think we want to have PointStyle as eventually the goal might be to have these point styles shared across a few different panels. I agree that maybe we need to update protobufEnumName to just be enumName. Also happy to make those changes

Initial pass at shape SDK work

84fe641

thswanson97 added 4 commits February 12, 2026 07:55

Update ordering on enums to provide a default of DOT if message was o…

94d6220

…ptional

Rename the enum values for clarity

e1fc7a2

Rename MarkerShape to PointStyle and update field name to point_style

88a9619

Add generated files for PointStyle name change

77f56e1

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 20:57 View deployment

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 21:00 View deployment

fix linter errors

9752020

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 22:05 View deployment

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 22:08 View deployment

Add serde support for optional enum fields

3603d24

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 22:24 View deployment

thswansonfxg marked this pull request as ready for review February 12, 2026 22:26

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 22:28 View deployment

Make C FFI optional enum use pointer for nullability, add serde round…

cd4da85

…-trip tests

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 22:40 View deployment

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 22:42 View deployment

cursor bot reviewed Feb 12, 2026

View reviewed changes

c/src/generated_types.rs Outdated Show resolved Hide resolved

typescript/schemas/src/internal/generateSdkCpp.ts Show resolved Hide resolved

thswanson97 and others added 2 commits February 12, 2026 14:49

Update generated files

72426ea

Merge branch 'main' into tylerswansonwt/ert-1612-add-optional-attribu…

3fb3be5

…te-to-control-point-shape-in-locationfix

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 22:52 View deployment

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 22:54 View deployment

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 22:55 View deployment

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 22:57 View deployment

cursor bot reviewed Feb 12, 2026

View reviewed changes

cpp/foxglove/src/schemas.cpp Outdated Show resolved Hide resolved

Fix UB in C++ to C optional enum conversion: use arena alloc + static…

2129942

…_cast

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 23:01 View deployment

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 23:03 View deployment

Regenerate C++ schemas with arena alloc for optional enum conversion

a5b2fa6

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 23:36 View deployment

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 23:38 View deployment

cursor bot reviewed Feb 12, 2026

View reviewed changes

rust/foxglove_proto_gen/src/lib.rs Show resolved Hide resolved

Add serde(default) for optional enum fields to handle missing keys du…

68d1522

…ring deserialization

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 23:56 View deployment

Regenerate foxglove.rs with serde(default) for optional enum fields

c7ecb4a

foxglovebot deployed to foxglove-sdk-playground (Preview) February 12, 2026 23:58 View deployment

github-actions bot deployed to foxglove-sdk-api-docs (Preview) February 12, 2026 23:59 View deployment

foxglovebot deployed to foxglove-sdk-playground (Preview) February 13, 2026 00:01 View deployment

thswansonfxg requested a review from mjsobrep February 13, 2026 00:06

thswansonfxg changed the title ~~Initial pass at shape SDK work~~ Add shape enums and optional values in sdk Feb 13, 2026

thswansonfxg requested review from defunctzombie, indirectlylit and jtbandes February 13, 2026 00:10

thswansonfxg changed the title ~~Add shape enums and optional values in sdk~~ Add optional shape enums in sdk Feb 13, 2026

thswansonfxg requested a review from ericmlujan February 13, 2026 00:45

jtbandes reviewed Feb 13, 2026

View reviewed changes

Conversation

thswansonfxg commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Docs

Description

Uh oh!

linear bot commented Feb 11, 2026

Uh oh!

claude bot commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Blockers

Suggestions

1. PR description should use "Fixes:" format for Linear link

2. Docs section is empty

3. Consider adding a test for the C FFI optional enum round-trip

4. Test fixture update is good, but snapshot diffs are noisy

5. Minor: color field uses reinterpret_cast in C++ but point_style uses arena allocation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtbandes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtbandes Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

thswansonfxg commented Feb 11, 2026 •

edited

Loading

claude bot commented Feb 12, 2026 •

edited

Loading

5. Minor: `color` field uses `reinterpret_cast` in C++ but `point_style` uses arena allocation

jtbandes Feb 13, 2026 •

edited

Loading